-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent autocompleter from letting users insert two More blocks #7166
Conversation
Make autocompleters respect the isDisabled property that's set by getInserterItems().
@@ -242,6 +250,10 @@ export class Autocomplete extends Component { | |||
const { open, range, query } = this.state; | |||
const { getOptionCompletion } = open || {}; | |||
|
|||
if ( option.isDisabled ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wondered why this was necessary due to the button actually being disabled. Now I see it is due to the fact that our keyboard listener calls select
with indiscriminately when ENTER is pressed. I wonder if it would be clearer to conditionally call select
in handleKeyDown
. Just sharing my experience in case you concur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we re-use select()
in both the keyDown
and onClick
handlers. I think it's OK to leave as is.
@@ -345,6 +357,7 @@ export class Autocomplete extends Component { | |||
value: optionData, | |||
label: completer.getOptionLabel( optionData ), | |||
keywords: completer.getOptionKeywords ? completer.getOptionKeywords( optionData ) : [], | |||
isDisabled: completer.isOptionDisabled ? completer.isOptionDisabled( optionData ) : false, | |||
} ) ); | |||
|
|||
const filteredOptions = filterOptions( this.state.search, keyedOptions ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be a pathological case, but since we show a limited number of options at a time, it is technically possible for all shown options to be disabled. Maybe we should update filterOptions
to sort disabled options to the end before filtering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really like this idea, but let's punt on it until disabled items are a more common occurrence. Right now it's really only the More block case that we have to support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this!
I left a couple of comments, but I don't think either should be blockers for getting this in.
Thanks Brandon! ❤️ |
Fixes #4225.
Makes autocompleters respect the
isDisabled
property that's set bygetInserterItems()
. This means that one can no longer insert two More blocks via the / autocompleter.How has this been tested?